fix: embedded omitempty field dropped / nil-pointer panic in nested embedded structs (#576, #581, #554) - #587
Open
truffle-dev wants to merge 3 commits into
Open
Conversation
When an embedded struct's first field has the omitempty tag and is empty,
the whole containing struct serialized to `{}`, dropping every following
field (issue goccy#576).
The cause is in encoder.StructCode.ToAnonymousOpcode: prevField was set to
firstField (the anonymous head opcode) instead of the last field of the
embedded chain, so the omitempty head's NextField never linked to the next
real field. addStructEndCode then wired that head's empty-skip jump straight
to StructEnd. Mirror the non-anonymous ToOpcode path and use lastFieldCode.
The nil-pointer panic in goccy#581 (embedded value struct followed by a nil omitempty pointer-to-struct field, itself embedded) shares the root cause with goccy#576: the omitempty field's NextField was never linked to the next field in ToAnonymousOpcode. The lastFieldCode change fixes both. Add TestIssue581 to guard the panic case.
Grandpa{Father{Son{B,C}, A []string omitempty}} panics with a nil
pointer dereference on master: an embedded struct followed by an
omitempty empty-slice field, double-nested. This is the same
ToAnonymousOpcode prevField wiring bug as goccy#576/goccy#581 - the embedded
head's NextField never links to the trailing omitempty field, so the
empty-skip jump lands on a nil target. The one-line lastFieldCode fix
resolves it. Verified: panics on master, clean on this branch.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #576. Also fixes the panic reported in #581 and #554.
When an embedded struct's first field is tagged
omitemptyand is empty, the whole containing struct serialized to{}, silently dropping every following field (#576). The same broken wiring also caused a nil-pointer panic for two closely related shapes (#581, #554).The cause is in
encoder.StructCode.ToAnonymousOpcode:prevFieldwas set tofirstField(the anonymous head opcode) rather than the last field of the embedded chain, so the omitempty field'sNextFieldnever linked to the next real field.addStructEndCodethen wired that head's empty-skip jump straight toStructEnd. The non-anonymousToOpcodepath already does this correctly vialastFieldCode; this mirrors it.Added
TestIssue576(empty / data-only / both-fields),TestIssue581(nil and set pointer), andTestIssue554(double-nested embedded with trailing omitempty empty slice). Each was confirmed to fail before the change ({"data":""}->{}for #576, panic for #581 and #554) and pass after. Fullgo test ./...is green, gofmt clean.For anyone triaging the embedded-omitempty cluster: I dump-tested the neighbours against this branch. #512 is a distinct shape (a plain field followed by an embedded struct whose own field is omitempty, double-nested) that goes through a separate opcode path and still panics; not addressed here. #486 (nil embedded self-pointer emitting
{"id":99,null}) and #503 (omitempty pointer child with a zero first byte) are also separate paths that survive this fix and remain open. Happy to follow those up in separate PRs.